draft: hiding filters showing no results (not working) #1403
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
packages/frontend/src/pages/Inbox/filters.ts (1)
Line range hint
77-155: Consider implementing proper filter dependenciesThe current approach of using
filteredDialogsto hide empty filters might not be the most maintainable solution. Consider implementing proper filter dependencies where:
- Each filter's options are dynamically updated based on other active filters
- The filtering logic is applied incrementally
- The UI clearly shows which filters are dependent on others
This would provide a better user experience and make the code more maintainable.
Would you like me to provide an example implementation of this approach?
packages/frontend/src/pages/Inbox/Inbox.tsx (1)
109-111: Remove commented out codeDead code should be removed rather than left commented out. If this code needs to be referenced later, it can be found in the git history.
- // const filterBarSettings = useMemo(() => { - // return getFilterBarSettings(dataSource, filteredDialogs, format); - // }, [dataSource, filteredDialogs]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/frontend/src/pages/Inbox/Inbox.tsx(1 hunks)packages/frontend/src/pages/Inbox/filters.ts(5 hunks)
🔇 Additional comments (3)
packages/frontend/src/pages/Inbox/filters.ts (2)
151-152: LGTM! This is the correct approach
This section correctly uses filteredDialogs for generating date options, which should be replicated in other filter sections.
92-96:
Fix inconsistent sender counts
The counts are calculated from the original dialogs array while options are generated from filteredDialogs. This means the counts won't reflect the actual available options after filtering, which could mislead users.
-const senders = dialogs.map((p) => p.sender.name);
+const senders = filteredDialogs.map((p) => p.sender.name);
const senderCounts = countOccurrences(senders);
const validSenders = filteredDialogs.map((p) => p.sender.name);
const uniqueSenders = Array.from(new Set(validSenders));Likely invalid or redundant comment.
packages/frontend/src/pages/Inbox/Inbox.tsx (1)
113-117:
Fix dependency array and implementation issues in filterBarSettings
There are several issues with this implementation:
-
The dependency array is missing
filteredDialogs, which could lead to stale filter options -
The current implementation filters out options with length ≤ 1, but this doesn't match the PR objective of "hiding filters showing no results"
-
The implementation might hide valid single-option filters that users still need to see
-
Fix the dependency array:
const filterBarSettings = useMemo(
() => getFilterBarSettings(dataSource, filteredDialogs, format).filter((setting) => setting.options.length > 1),
- [dataSource, format],
+ [dataSource, filteredDialogs, format],
);- Consider revising the implementation to align with the PR objective:
const filterBarSettings = useMemo(
- () => getFilterBarSettings(dataSource, filteredDialogs, format).filter((setting) => setting.options.length > 1),
+ () => {
+ const settings = getFilterBarSettings(dataSource, filteredDialogs, format);
+ return settings.filter((setting) => {
+ // Only hide filters that would yield no results when applied
+ const hasMatchingDialogs = filteredDialogs.some(dialog =>
+ setting.options.some(option =>
+ dialog[setting.key] === option.value
+ )
+ );
+ return hasMatchingDialogs;
+ });
+ },
[dataSource, filteredDialogs, format],
);Let's verify the filter behavior:
|
|
||
| const validReceivers = filteredDialogs.map((p) => p.receiver.name); | ||
| const uniqueReceivers = Array.from(new Set(validReceivers)); | ||
|
|
||
| return uniqueReceivers.map((receiver) => ({ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inconsistent receiver counts and reduce code duplication
- The counts are calculated from the original
dialogsarray, causing the same inconsistency issue as the sender filter. - The logic for generating options is duplicated between sender and receiver filters.
First, fix the counting:
-const receivers = dialogs.map((p) => p.receiver.name);
+const receivers = filteredDialogs.map((p) => p.receiver.name);
const receiversCount = countOccurrences(receivers);Then, consider extracting a helper function to reduce duplication:
const getParticipantOptions = (
dialogs: InboxItemInput[],
participantType: 'sender' | 'receiver',
labelPrefix: string
) => {
const participants = dialogs.map((p) => p[participantType].name);
const counts = countOccurrences(participants);
const unique = Array.from(new Set(participants));
return unique.map((name) => ({
displayLabel: `${t(labelPrefix)} ${name}`,
value: name,
count: counts[name],
}));
};| export const getFilterBarSettings = ( | ||
| dialogs: InboxItemInput[], | ||
| filteredDialogs: InboxItemInput[], | ||
| format: FormatFunction, | ||
| ): FilterSetting[] => { |
There was a problem hiding this comment.
Consider using only filteredDialogs for consistency
The function now takes both dialogs and filteredDialogs, but this creates potential inconsistencies in the counts vs. available options. Consider using only filteredDialogs for both counting and option generation to ensure consistency.
export const getFilterBarSettings = (
- dialogs: InboxItemInput[],
filteredDialogs: InboxItemInput[],
format: FormatFunction,
): FilterSetting[] => {Committable suggestion skipped: line range outside the PR's diff.
| const statuses = dialogs.map((p) => p.status); | ||
| const statusCounts = countOccurrences(statuses); | ||
|
|
||
| const validStatuses = filteredDialogs.map((p) => p.status); | ||
| const uniqueStatuses = Array.from(new Set(validStatuses)); | ||
|
|
||
| return uniqueStatuses.map((statusLabel) => ({ | ||
| displayLabel: t(`status.${statusLabel.toLowerCase()}`), | ||
| value: statusLabel, | ||
| count: statusCount[statusLabel], | ||
| count: statusCounts[statusLabel], |
There was a problem hiding this comment.
💡 Codebase verification
Based on the results from our verification attempts, I can generate the final response:
Fix status counts and verify translation key case handling
The status counts issue needs to be fixed as proposed in the diff, but regarding the translation keys, we found evidence in the translation files (nb.json and en.json) that the lowercase status keys are correct. The translation files contain entries like "status.completed", "status.draft", etc., which match the lowercase transformation in the code.
- Fix the counting inconsistency by using
filteredDialogsinstead ofdialogs - ✓ Translation key case handling is correct as-is
🔗 Analysis chain
Fix status counts and verify translation key case handling
- The status counts have the same inconsistency issue as other filters.
- The translation key uses lowercase which might not match the actual translation keys.
-const statuses = dialogs.map((p) => p.status);
+const statuses = filteredDialogs.map((p) => p.status);
const statusCounts = countOccurrences(statuses);Let's verify the translation keys:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if translation keys exist in lowercase
rg -l "status\.: \{" . | xargs rg "status\.[A-Z]+" -A 1
Length of output: 57
Script:
#!/bin/bash
# Let's try a different approach to find translation files and keys
fd -e json -e yaml -e yml | xargs rg "status\." -B 1 -A 1
# Also search for translation related files
fd "i18n|translation|locale" --type f
# Look for actual status values in the code
rg "status: ['\"](.*?)['\"]" --type ts
Length of output: 4105
Script:
#!/bin/bash
# Let's find the actual status values used in the dialogs
rg "status: ['\"]" packages/frontend/src/pages/Inbox -B 2 -A 2
# Also check how these status values are used in the code
ast-grep --pattern 'status.$_'
Length of output: 99
Script:
#!/bin/bash
# Let's try to find the Dialog type definition to see status values
rg "type.*Dialog.*status" --type ts -A 5
# And check for enum definitions related to status
rg "enum.*Status" --type ts -A 10
# Also look for actual status assignments in the Inbox directory
rg "\.status\s*(=|:)" packages/frontend/src/pages/Inbox
Length of output: 136
| const filteredDialogs = useMemo(() => { | ||
| return filterDialogs(dataSource, activeFilters, format); | ||
| }, [dataSource, activeFilters]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate memoized variable
The filteredDialogs variable duplicates the exact same logic as itemsToDisplay defined above. This creates unnecessary memory overhead and complexity.
Remove this duplicate and use the existing itemsToDisplay variable instead:
- const filteredDialogs = useMemo(() => {
- return filterDialogs(dataSource, activeFilters, format);
- }, [dataSource, activeFilters]);
Hva er endret?
Related Issue(s)
Dokumentasjon / Storybook / testdekning
storiesiStorybook, eller så er ikke dette relevant.Skjermbilder eller GIFs (valgfritt)
Summary by CodeRabbit
New Features
Bug Fixes